-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Async testing #21
Async testing #21
Conversation
@@ -55,6 +36,11 @@ class App extends Component { | |||
/> | |||
</Col> | |||
</Row> | |||
<Row className="d-flex justify-content-center"> | |||
<Col md="auto"> | |||
{this.state.error && <Alert variant="danger">{this.state.error}</Alert>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conditional rendering ftw!
|
||
const returnHome = jest.fn(); | ||
|
||
describe("NavBar", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
future note: For Iteration 5, we could add a search input into the navbar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add some SCSS for the NavBar and Selection Movie.
Aside form that though, it looks/acts marvelous... absolutely marvelous!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you also need an integration test that checks that when a movie is clicked, the details are rendered! My guess is that App will need to do this test.
@@ -13,31 +13,12 @@ class App extends Component { | |||
super(); | |||
this.state = { | |||
movies: [], | |||
selectedMovie: 0, | |||
selectedMovie: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the initial value for this be an empty object instead of null? Always best to avoid mutating the data type.
@@ -91,6 +77,25 @@ class App extends Component { | |||
</React.Fragment> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun fact: you can create a react fragment with the shorthand <> </>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is a fun fact!
componentDidMount() { | ||
getMovies() | ||
.then((response) => this.setState({ movies: response.movies })) | ||
.then(this.setState({ loader: false })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the loading message is dependent solely on if there are movies rendered or not, we can remove this part of state and just check on this.state.moves! However if more than one condition can set this to true, then it can stay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duly noted!
getMovies() | ||
.then((response) => this.setState({ movies: response.movies })) | ||
.then(this.setState({ loader: false })) | ||
.catch((err) => this.setState({ error: err })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be sure that "err" is a string as the initial state indicates, and not an object with an error message inside it.
|
||
returnToHome = (event) => { | ||
this.setState({ | ||
selectedMovie: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the default value of selectedMovie should be 0 instead of null!
|
||
// should be able to click on a movie card |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I LOVE the pseudocoding! This is a great workflow!
|
||
userEvent.click(movieCardPoster); | ||
|
||
expect(handleClick).toHaveBeenCalled(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great unit test
What does this PR do?
Where should the reviewer start?
How should this be manually tested?
What are the relevant tickets?